-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Backport the MS SQL Client to Vert.x 3.9.x #604
Conversation
Just adapt the code from the 4.x branch (master) to work with 3.9.x. Signed-off-by: Clement Escoffier <[email protected]>
Oh, that's great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Can you also add this stage to the .travis.yml
file though so we have test coverage right away and make sure that all tests are passing?
- stage: test
name: "MSSQL 2017-CU12"
jdk: openjdk8
script: mvn -q clean verify -B --projects vertx-sql-client,vertx-mssql-client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a personal note, I am not currently in favor of this back-port because it is lacking some important features. These features will almost for sure let users down and we will get more maintenance burden on the backport, than focusing on getting the current master and 4.0.0 out of the door.
- [MSSQL]TLS support #605 - without this, most likely you won't be able to use the client on a cloud environment such as Azure, or on perm where the application server and db server are not the same physical machine.
- [MSSQL]Cursor support #606 - missing cursors, will likely be a blocker for stored procedure users or "power users" that stream results from the db directly.
- [MSSQL]Batch Queries support #607 - this can be a blocker for ETL applications where batching is a common technique to address large ingestion of data.
- [MSSQL]datatype support #608 - A more generic issue the current code only supports a subset of the datatypes MSSQL has. Unlike
pg
andmysql
where we have (if not all) data types handled. - [MSSQL]Transaction support #610 - Most definitely a blocker, missing transactions will be a blocker unless users are just thinkering or PoC projects where data loss can be allowed/developing simple read only applications.
This back-port will create more maintenance work for the team which is not able to sustain this load. We could think of a back-port in the future if the expectations are clearly defined. I'd suggest as an alternative that downtream projects interested on this module to start using the SNAPSHOT
builds on sonatype or the milestone releases instead.
The downstream cannot use Vert.x 4.0 milestone or snapshots before the end
of the year. So, without a backport an alternative would need to be found.
Le ven. 8 mai 2020 à 09:54, Paulo Lopes <[email protected]> a écrit :
… ***@***.**** commented on this pull request.
As a personal note, I am not currently in favor of this back-port because
it is lacking some important features. These features will almost for sure
let users down and we will get more maintenance burden on the backport,
than focusing on getting the current master and 4.0.0 out of the door.
- #605 <#605>
- without this, most likely you won't be able to use the client on a cloud
environment such as Azure, or on perm where the application server and db
server are not the same physical machine.
- #606 <#606>
- missing cursors, will likely be a blocker for stored procedure users or
"power users" that stream results from the db directly.
- #607 <#607>
- this can be a blocker for ETL applications where batching is a common
technique to address large ingestion of data.
- #608 <#608>
- A more generic issue the current code only supports a subset of the
datatypes MSSQL has. Unlike pg and mysql where we have (if not all)
data types handled.
- #610 <#610>
- Most definitely a blocker, missing transactions will be a blocker unless
users are just thinkering or PoC projects where data loss can be
allowed/developing simple read only applications.
This back-port will create more maintenance work for the team which is not
able to sustain this load. We could think of a back-port in the future if
the expectations are clearly defined. I'd suggest as an alternative that
downtream projects interested on this module to start using the SNAPSHOT
builds on sonatype or the milestone releases instead.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#604 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADCG7J33RZKXXJI2MZFQJTRQO3ERANCNFSM4M2GRTUA>
.
|
@pmlopes are any of those issues blockers for typical usage of the Vert.x client under Hibernate Reactive and especially with Quarkus? (This use case is the motivation for the backport.) It doesn't look to me like they are, though I'm not sure about #605.
That's not an option for Quarkus, and therefore also not an option for Hibernate Reactive. |
@gavinking can you use test this branch to see how much it fits your current expectations ? |
@vietj I can try it out in standalone mode, I guess, though I'm not the right person to test the Quarkus scenario. |
Alright, so I tried to get a very basic test ( https://github.com/gavinking/hibernate-rx/tree/sqlserver I can see a couple of queries for next sequence values executed and returning successfully, but then as soon as I try to run the first I have no clue what the cause might be: something to do with us not using with transactions? Something to do with the connection properties I'm using? Something else? |
@gavinking can you make a reproducer case ? e.g you add a new test in the sql server tests and then we get an analysis from @BillyYccc |
Folks, I have simplified it down to the following: package org.hibernate.rx;
import io.vertx.core.Vertx;
import io.vertx.ext.unit.Async;
import io.vertx.ext.unit.TestContext;
import io.vertx.ext.unit.junit.VertxUnitRunner;
import io.vertx.mssqlclient.MSSQLConnectOptions;
import io.vertx.mssqlclient.MSSQLPool;
import io.vertx.sqlclient.PoolOptions;
import org.junit.Test;
import org.junit.runner.RunWith;
@RunWith(VertxUnitRunner.class)
public class ConnectionTest {
@Test
public void testConnection(TestContext context) {
MSSQLConnectOptions options = new MSSQLConnectOptions()
.setHost( "localhost" )
.setUser( "sa" )
.setPassword( "reallyStrongPwd123" );
MSSQLPool pool = MSSQLPool.pool( Vertx.vertx(), options, new PoolOptions() );
Async async = context.async();
pool.preparedQuery( "select * from INFORMATION_SCHEMA.SEQUENCES" ).execute(
ar -> {
System.out.println( ar.result().rowCount() );
System.out.println("...done");
async.complete();
}
);
System.out.println("waiting...");
}
} This test hangs and never completes. Surely this must be something simple somehow related to my setup. |
But the thing that seems strange is that if I replace the query with:
Then the test completes successfully. Which is why I wonder if this has something to do with txns. |
so you mean that "select * from INFORMATION_SCHEMA.SEQUENCES" has a problem ? |
Yes, but any table will do. It's not specific to |
I have tried and found some of the data types in the table are not supported for now such as nvarchar. |
It's not just that. Honestly I'm just finding a whole lot of really strange behavior (queries that sometimes work and sometimes don't). But it's really hard to debug because I never get any error. I think the root problem is that it shouldn't hang when an error occurs. It should send the error back to my callback and/or terminate the async operation. This doesn't seem to happen: I never get errors at all, no matter how broken my query is. I can send the query |
OK, so apart from the Big problem, which is the failure to report errors, it looks like the other major source of my pain is that it doesn't like inserting Here's the test code I'm using to show this. Note that it uses testcontainers to create the db. package org.hibernate.rx;
import io.vertx.core.Vertx;
import io.vertx.ext.unit.Async;
import io.vertx.ext.unit.TestContext;
import io.vertx.ext.unit.junit.VertxUnitRunner;
import io.vertx.mssqlclient.MSSQLConnectOptions;
import io.vertx.mssqlclient.MSSQLPool;
import io.vertx.sqlclient.PoolOptions;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.testcontainers.containers.MSSQLServerContainer;
@RunWith(VertxUnitRunner.class)
public class ConnectionTest {
public static final MSSQLServerContainer<?> sqlserver = new MSSQLServerContainer<>()
.withReuse( true );
@Test
public void testConnection(TestContext context) {
sqlserver.start();
System.out.println( sqlserver.getJdbcUrl() );
System.out.println( sqlserver.getFirstMappedPort() );
MSSQLConnectOptions options = new MSSQLConnectOptions()
.setHost( "localhost" )
.setPort( sqlserver.getFirstMappedPort() )
.setUser( sqlserver.getUsername() )
.setPassword( sqlserver.getPassword() );
MSSQLPool pool = MSSQLPool.pool( Vertx.vertx(), options, new PoolOptions() );
Async async = context.async();
pool.preparedQuery( "create table WithNull (nullint int)" ).execute(
ar1 -> {
System.out.println( ar1.result().rowCount() );
System.out.println("...inserting...");
pool.preparedQuery("insert into WithNull (nullint) values (null)").execute(
ar2 -> {
System.out.println( ar2.result().rowCount() );
System.out.println("...querying...");
pool.preparedQuery("select * from WithNull").execute(
ar3 -> {
System.out.println( ar3.result().rowCount() );
System.out.println("...done");
sqlserver.stop();
async.complete();
}
);
}
);
}
);
System.out.println("creating table...");
}
} |
I have create an issue for this, see #627. |
Just adapt the code from the 4.x branch (master) to work with 3.9.x.
Motivation:
There is a need to get the MS SQL client to work against 3.9. This PR is just adapting the code to follow the Vert.x 3.9 conventions.